feat: use gRPC for queries #10997
feat: use gRPC for queries #10997
Conversation
|
@AmauryM in continue to our discord discussion. this is rather PoC which seems to work |
troian
force-pushed
the
issue-10996
branch
3 times, most recently
from
6d7302a
to
4389f2d
Compare
troian
force-pushed
the
issue-10996
branch
3 times, most recently
from
73f3eb4
to
d41ebfb
Compare
|
some updates to the PR.
questions.
rpcURI, _ := flagSet.GetString(flags.FlagNode)
if clientCtx.Client == nil || flagSet.Changed(flags.FlagNode) {
if rpcURI != "" {
clientCtx = clientCtx.WithNodeURI(rpcURI)
client, err := NewClientFromNode(rpcURI)
if err != nil {
return clientCtx, err
}
clientCtx = clientCtx.WithClient(client)
}
}
isOffline, _ := flagSet.GetBool(flags.FlagOffline)
if !isOffline && clientCtx.ClientConn == nil {
grpcURI, _ := flagSet.GetString(flags.FlagGRPC)
if grpcURI != "" {
if !flagSet.Changed(flags.FlagGRPC) && rpcURI != "" {
if uri, err := url.Parse(rpcURI); err == nil {
guri, _ := url.Parse(grpcURI)
grpcURI = uri.Hostname() + ":" + guri.Port()
}
}
if gcl, err := grpc.DialContext(ctx, grpcURI, grpc.WithInsecure(), grpc.WithReturnConnectionError()); err == nil {
clientCtx = clientCtx.WithClientConn(gcl)
}
}
} |
to replicate current behaviour I think it should be something like default to rpc, unless --grpc is passed. Defaulting to localhost here is fine.
Making the user explicitly set a grpc endpoint there I think is best. There are many endpoint that expose the RPC but not grpc and vice versa. |
|
do-not-merge
I tend to option 2 as the flag on the client side will be often used by people so they have to type less. As GRPC is not yet widely adopted in cosmos ecosystem, renaming server block to |
|
i personally prefer option 1, because it's keep backwards compatibility. Renaming app.toml fields requires its own migration path, which is more effort. Also, on the client side, we already have the |
|
we have a weird race in tests happening when gRPC is on. that's with cosmos v0.44.5. Any thoughts? it happens on the network cleanup |
|
@troian let me know how I can help push this over the finish line. Secret wants to test this to see if it fixes something else. |
fixes #10996 Signed-off-by: Artur Troian <troian.ap@gmail.com>
There was a problem hiding this comment.
Hey @troian, do you think this is ready for review?
For me it looks good, maybe #10997 (comment) is the only comment I have left
|
@AmauryM this is the race we're seeing. considering we're on the also running real testnet with clients using grpc reproduces the issue |
|
@AmauryM thoughts on the last comment? |
|
Oh wow, it seems like you found a server-side bug that's in production. It was maybe just that no-one ever used the gRPC endpoint directly, that's why we never saw this issue. @troian Would you like to add a mutex on app.checkState? I can also do this tomorrow (maybe it's cleaner to do the mutex in another PR) |
|
yeah, i have pr in my drafts. still doing some tests to make sure it work |
this is with this branch being used? I know people running queries in parallel and this is the first time seeing this |
I believe 0.44.5 as #10997 (comment). Is there a possibility they all used |
I think this is the case, which explains why teams like terra and secret were running into non parallel queries when trying to do it |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| grpcURI, _ := flagSet.GetString(flags.FlagGRPCNode) | ||
|
|
||
| if grpcURI != "" { | ||
| grpcKeepAlive, _ := flagSet.GetDuration(flags.FlagGRPCKeepAlive) |
| @@ -147,6 +150,23 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont | |||
| } | |||
| } | |||
|
|
|||
| if clientCtx.ClientConn == nil || flagSet.Changed(flags.FlagGRPCNode) { | |||
| grpcURI, _ := flagSet.GetString(flags.FlagGRPCNode) | |||
| if err == nil { | ||
| clientCtx = clientCtx.WithClientConn(gcl) | ||
| } |
There was a problem hiding this comment.
Prefer returning a zero-value Context when the error is non-nil.
| @@ -91,6 +94,8 @@ var LineBreak = &cobra.Command{Run: func(*cobra.Command, []string) {}} | |||
| // AddQueryFlagsToCmd adds common flags to a module query command. | |||
| func AddQueryFlagsToCmd(cmd *cobra.Command) { | |||
| cmd.Flags().String(FlagNode, "tcp://localhost:26657", "<host>:<port> to Tendermint RPC interface for this chain") | |||
| cmd.Flags().String(FlagGRPCNode, "", "<host>:<port> to GRPC interface for this chain") | |||
| cmd.Flags().Duration(FlagGRPCKeepAlive, 20*time.Second, "set GRPC keepalive. defaults to 20s") | |||
There was a problem hiding this comment.
No need to re-iterate the default value in the help text, as it is printed anyway.
|
we reopening this as it's needed for Ethermint |
|
LFG |
|
@marbar3778 this issue needs to be addressed first #11102 |
Correct.. I patched the 0.45.2 release with this PR to test this on Ethermint and I am currently seeing the following error |
|
@crypto-facs highly possible. last time i worked on the pr more than a month ago. |
|
@marbar3778 @crypto-facs i know that oKEX forked the SDK and added this functionality. We should look into their implementation |
|
@alexanderbez sounds good. closing |
fixes #10996
Signed-off-by: Artur Troian troian.ap@gmail.com